AAP-70294: Migrate Unit Test Candidates from ATF to Upstream#16385
AAP-70294: Migrate Unit Test Candidates from ATF to Upstream#16385jessicamack wants to merge 3 commits intoansible:develfrom
Conversation
📝 WalkthroughWalkthroughThree new functional tests were added to verify API constraint behaviors: inventory names must be unique per organization, notification template names must be unique per organization, and bulk job launch requests respect a configurable system limit setting. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
awx/main/tests/functional/test_bulk.py (1)
2-2: Unused import.The
mockimport is not used anywhere in the file.🧹 Proposed fix
-from unittest import mock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/functional/test_bulk.py` at line 2, Remove the unused import "mock" from the top of the test file: delete the "from unittest import mock" import statement (the only reference is the symbol "mock") so the module no longer contains an unused import; run tests to verify nothing else depends on "mock" in this file.awx/main/tests/functional/api/test_notification_templates.py (1)
1-92: LGTM!Well-structured test that mirrors the inventory uniqueness test pattern. The test correctly validates the organization-scoped uniqueness constraint for notification template names, covering both allowed (cross-org) and rejected (same-org duplicate) scenarios.
Minor optional suggestion: The
notification_configurationdict is repeated three times. Consider extracting it to a local variable to reduce duplication:notification_config = { 'username': 'user@example.com', 'password': 'pass', 'sender': 'sender@example.com', 'recipients': ['recipient@example.com'], 'host': 'smtp.example.com', 'port': 25, 'use_tls': False, 'use_ssl': False, }This is purely a readability/maintainability improvement and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/functional/api/test_notification_templates.py` around lines 1 - 92, Extract the repeated notification_configuration dict used in test_notification_template_names_unique_per_organization into a single local variable (e.g., notification_config) and reuse it in the three post(...) calls to reduce duplication and improve readability; update each post payload to pass that variable for the 'notification_configuration' key and keep all other fields and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/tests/functional/api/test_notification_templates.py`:
- Around line 1-92: Extract the repeated notification_configuration dict used in
test_notification_template_names_unique_per_organization into a single local
variable (e.g., notification_config) and reuse it in the three post(...) calls
to reduce duplication and improve readability; update each post payload to pass
that variable for the 'notification_configuration' key and keep all other fields
and assertions unchanged.
In `@awx/main/tests/functional/test_bulk.py`:
- Line 2: Remove the unused import "mock" from the top of the test file: delete
the "from unittest import mock" import statement (the only reference is the
symbol "mock") so the module no longer contains an unused import; run tests to
verify nothing else depends on "mock" in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dcc5e42-6c3c-4ed3-9993-ac7a69bb6fc3
📒 Files selected for processing (3)
awx/main/tests/functional/api/test_inventory.pyawx/main/tests/functional/api/test_notification_templates.pyawx/main/tests/functional/test_bulk.py
tvo318
left a comment
There was a problem hiding this comment.
I think everything else looks alright.. once you commit the changes, let's see how the CI checks go and I can approve.
adrisala
left a comment
There was a problem hiding this comment.
Moving my comments here. I'm missing some tests from ATF:
- I don't see anything for test_pagination
- For test_bulk_api I see the test added bears no relation with the tests in the ATF file (test_bulk_job_launch_respects_settings_limit and the ATF test had test_create_hosts, test_launch_jobs, test_create_multiple_jobs_after_modifying_settings
Are these tested somewhere else in the tower repo? I want to be sure we're not losing coverage with this
|
b2103dc to
879bd22
Compare
|



SUMMARY
There are tests in controller-test-suite that are currently written as integration tests but can be rewritten as unit/functional tests in tower. This PR is adding some of those identified tests.
ISSUE TYPE
COMPONENT NAME
Summary by CodeRabbit